Skip to content

Add LocalVariableNameFactory. #3271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: 4.0.x
Choose a base branch
from
Open

Add LocalVariableNameFactory. #3271

wants to merge 5 commits into from

Conversation

christophstrobl
Copy link
Member

Add a variable name factory that considers predefined names and resolves name clashes.
Expose variable name clash resolution via the generation context of a single method.

Copy link
Contributor

@onobc onobc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @christophstrobl . I made a few minor suggestions. PTAL.

return new LocalVariableNameFactory(variables);
}

LocalVariableNameFactory(Iterable<String> predefinedVariableNames) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A simplification of this could be:

	LocalVariableNameFactory(Iterable<String> predefinedVariableNames) {
		predefinedVariableNames.forEach((paramName) -> variables.put(paramName, 0L));
	}

Wdyt?

Add a variable name factory that considers predefined names and resolves name clashes.
Expose variable name clash resolution via the generation context of a single method.
}

String targetName = suggestTargetName(intendedVariableName);
variables.add(intendedVariableName, targetName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to update the original intendedVariableName?

This code is fine but concurrency aside, I still think something like the following that would replace line62-86 would simplify things:

@Override
public String generateName(String suggestedName) {
	var counter = variables.compute(suggestedName,
			(__, currentCount) -> currentCount == null ? 0 : currentCount.longValue() + 1);
	return counter == 0 ? suggestedName : "%s_%s".formatted(suggestedName, counter));
}

Copy link
Contributor

@onobc onobc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @christophstrobl - LGTM

* @param variableName the intended variable name.
* @return the suggested VariableName
*/
public String suggestLocalVariableName(String variableName) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest refining the way we approach the problem. We generally hold details in the context and can look these up. I would suggest that we keep a map around with parameter name mappings and use this method as getLocalVariableName(…) or localVariable(String logicalName). One could then call the method with the same name twice. On the first call, we would ensure there is no clash, the second call would return the mapped name. Would also alleviate the need to track the variable name in the calling code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry I don't get that comment. Why would I want to call the same method twice?
you'll have to keep track of variable name in the calling code since you might have to apply it in different code blocks or have to pass it around between query creation and execution so you refer to right thing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you'll have to keep track of variable name in the calling code since you might have to apply it in different code blocks or have to pass it around between query creation and execution so you refer to right thing.

Calling code would then not be required to track the actual variable name but could refer to the context so the context provides the information.

*/
class MethodMetadata {

private final Map<String, ParameterSpec> methodArguments = new LinkedHashMap<>();
private final ResolvableType actualReturnType;
private final ResolvableType returnType;

public MethodMetadata(RepositoryInformation repositoryInformation, Method method) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I liked the visibility modifiers to indicate which methods are desired API and to differentiate between those ones, that are package-private for testing purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants